-
Notifications
You must be signed in to change notification settings - Fork 109
Add frame processor support for audio streams #533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
ladvoc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM ✅
davidzhao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lg
| owned_buffer_info = audio_event.frame_received.frame | ||
| frame = AudioFrame._from_owned_info(owned_buffer_info) | ||
| if self._processor is not None: | ||
| frame = self._processor._process(frame) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
neat.. this is clean
theomonnom
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
| @property | ||
| @abstractmethod | ||
| def is_enabled(self) -> bool: ... | ||
|
|
||
| @abstractmethod | ||
| def set_enabled(self, enable: bool): ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if using a property would work? It seems more python idiomatic
@property
@abstractmethod
def enabled(self) -> bool: ...
@enabled.setter
@abstractmethod
def enabled(self, value: bool) -> None: ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be used like:
processor.enabled = False
processor.enabled = True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds fine to me!
| @abstractmethod | ||
| def _update_stream_info( | ||
| self, | ||
| *, | ||
| room_name: str, | ||
| participant_identity: str, | ||
| publication_sid: str, | ||
| ): ... | ||
|
|
||
| @abstractmethod | ||
| def _update_credentials(self, *, token: str, url: str): ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If this is called by an external event, maybe we can call those events like this?
So it is obvious for the users implementing them that this is external.
def on_stream_info_updated()
def on_credentials_updated()
| def _process(self, frame: T) -> T: ... | ||
|
|
||
| @abstractmethod | ||
| def _close(self): ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's OK for close to be public
| def _close(self): ... | |
| def close(self): ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking close would rather be an internal method that we call to end the lifecycle of a processor.
Users would be expected to disable it with processor.enabled = False.
Co-authored-by: Théo Monnom <theo.8bits@gmail.com>
No description provided.